Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework ptr-to-ref conversion suggestion for method calls #123007

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

kadiwa4
Copy link
Contributor

@kadiwa4 kadiwa4 commented Mar 24, 2024

If we have a value z of type *const u8 and try to call z.to_string(), the upstream compiler will show you a note suggesting to call <*const u8>::as_ref first.

This PR extends that:

  • The note will only be shown when the method would exist on the corresponding reference type
  • It can now suggest any of <*const u8>::as_ref, <*mut u8>::as_ref and <*mut u8>::as_mut, depending on what the method needs.

I didn't introduce a help message because that's not a good idea with unsafe functions (and you'd also need to unwrap the Option<&_> somehow).
People should check the safety requirements.

For the simplest case

fn main() {
    let x = 8u8;
    let z: *const u8 = &x;
    // issue #21596
    println!("{}", z.to_string()); //~ ERROR E0599
}

the output changes like this:

 error[E0599]: `*const u8` doesn't implement `std::fmt::Display`
   --> $DIR/suggest-convert-ptr-to-ref.rs:5:22
    |
 LL |     println!("{}", z.to_string());
    |                      ^^^^^^^^^ `*const u8` cannot be formatted with the default formatter
    |
-   = note: try using `<*const T>::as_ref()` to get a reference to the type behind the pointer: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_ref
-   = note: using `<*const T>::as_ref()` on a pointer which is unaligned or points to invalid or uninitialized memory is undefined behavior
+note: the method `to_string` exists on the type `&u8`
+  --> $SRC_DIR/alloc/src/string.rs:LL:COL
+   = note: you might want to use the unsafe method `<*const T>::as_ref` to get an optional reference to the value behind the pointer
+   = note: read the documentation for `<*const T>::as_ref` and ensure you satisfy its safety preconditions before calling it to avoid undefined behavior: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_ref
    = note: the following trait bounds were not satisfied:
            `*const u8: std::fmt::Display`
            which is required by `*const u8: ToString`

I removed the separate note about the safety requirements because it was incomplete and the linked doc page already has the information you need.

Fixes #83695, but that's more of a side effect. The upstream compiler already suggests the right method name here.

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Mar 24, 2024
&& let ty::RawPtr(ty, ptr_mutbl) = *rcvr_ty.kind()
&& let Ok(pick) = self.lookup_probe_for_diagnostic(
item_name,
Ty::new_ref(tcx, ty::Region::new_error_misc(tcx), ty, ptr_mutbl),
Copy link
Contributor Author

@kadiwa4 kadiwa4 Mar 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is the right lifetime to use.

@kadiwa4
Copy link
Contributor Author

kadiwa4 commented Mar 24, 2024

r? compiler

@rustbot rustbot assigned pnkfelix and unassigned nnethercote Mar 24, 2024
@bors
Copy link
Contributor

bors commented Mar 25, 2024

☔ The latest upstream changes (presumably #123036) made this pull request unmergeable. Please resolve the merge conflicts.

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from d3da599 to b5e7a1c Compare March 25, 2024 19:57
@bors
Copy link
Contributor

bors commented Apr 3, 2024

☔ The latest upstream changes (presumably #123429) made this pull request unmergeable. Please resolve the merge conflicts.

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from b5e7a1c to 18f4f3d Compare April 3, 2024 23:23
@estebank
Copy link
Contributor

estebank commented Apr 4, 2024

I removed the separate note about the safety requirements because it was incomplete and the linked doc page already has the information you need.

TBH, I would change it to be

+note: the method `to_string` exists on the type `&u8`
+  --> $SRC_DIR/alloc/src/string.rs:LL:COL
+   = note: you might want to use the unsafe method `<*const T>::as_ref` to get an optional reference to the value behind the pointer
+   = warning: using `<*const T>::as_ref()` on a pointer which is unaligned or points to invalid or uninitialized memory is undefined behavior: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_ref

or

+   = warning: read the documentation for <*const T>::as_ref() and ensure you satisfy its safety preconditions before calling it to avoid undefined behavior: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_ref

In general, we can rely on links to have information for things that are nice to know but nor absolutely required. Most people barely read the errors, even fewer read the linked docs.

If we reliably had <blink> on the terminal, I'd be tempted to use it for this 😛

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch 2 times, most recently from cd3bf4f to c78d878 Compare April 4, 2024 17:53
@rust-log-analyzer

This comment has been minimized.

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from c78d878 to 57aa6b1 Compare April 4, 2024 19:24
@bors
Copy link
Contributor

bors commented Apr 6, 2024

☔ The latest upstream changes (presumably #123339) made this pull request unmergeable. Please resolve the merge conflicts.

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from 57aa6b1 to 41676ff Compare April 6, 2024 18:51
@bors
Copy link
Contributor

bors commented Apr 7, 2024

☔ The latest upstream changes (presumably #123576) made this pull request unmergeable. Please resolve the merge conflicts.

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from 41676ff to 0d20a84 Compare April 7, 2024 08:24
@bors
Copy link
Contributor

bors commented Apr 8, 2024

☔ The latest upstream changes (presumably #123569) made this pull request unmergeable. Please resolve the merge conflicts.

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from 0d20a84 to c214d4b Compare April 8, 2024 06:12
@bors
Copy link
Contributor

bors commented Apr 10, 2024

☔ The latest upstream changes (presumably #123708) made this pull request unmergeable. Please resolve the merge conflicts.

@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from c214d4b to e53c449 Compare April 10, 2024 09:07
src/doc/book Outdated Show resolved Hide resolved
@kadiwa4 kadiwa4 force-pushed the suggest_convert_ptr_to_mut_ref branch from e53c449 to 3a2a3ae Compare April 10, 2024 16:51
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2024

📌 Commit 3a2a3ae has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2024
@bors
Copy link
Contributor

bors commented Apr 11, 2024

⌛ Testing commit 3a2a3ae with merge e1fa18f...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
…ef, r=estebank

Rework ptr-to-ref conversion suggestion for method calls

If we have a value `z` of type `*const u8` and try to call `z.to_string()`, the upstream compiler will show you a note suggesting to call `<*const u8>::as_ref` first.

This PR extends that:
- The note will only be shown when the method would exist on the corresponding reference type
- It can now suggest any of `<*const u8>::as_ref`, `<*mut u8>::as_ref` and `<*mut u8>::as_mut`, depending on what the method needs.

I didn't introduce a `help` message because that's not a good idea with `unsafe` functions (and you'd also need to unwrap the `Option<&_>` somehow).
People should check the safety requirements.

For the simplest case
```rust
fn main() {
    let x = 8u8;
    let z: *const u8 = &x;
    // issue rust-lang#21596
    println!("{}", z.to_string()); //~ ERROR E0599
}
```
the output changes like this:
```diff
 error[E0599]: `*const u8` doesn't implement `std::fmt::Display`
   --> $DIR/suggest-convert-ptr-to-ref.rs:5:22
    |
 LL |     println!("{}", z.to_string());
    |                      ^^^^^^^^^ `*const u8` cannot be formatted with the default formatter
    |
-   = note: try using `<*const T>::as_ref()` to get a reference to the type behind the pointer: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_ref
-   = note: using `<*const T>::as_ref()` on a pointer which is unaligned or points to invalid or uninitialized memory is undefined behavior
+note: the method `to_string` exists on the type `&u8`
+  --> $SRC_DIR/alloc/src/string.rs:LL:COL
+   = note: try using the unsafe method `<*const T>::as_ref` to get an optional reference to the value behind the pointer: https://doc.rust-lang.org/std/primitive.pointer.html#method.as_ref
    = note: the following trait bounds were not satisfied:
            `*const u8: std::fmt::Display`
            which is required by `*const u8: ToString`
```

I removed the separate note about the safety requirements because it was incomplete and the linked doc page already has the information you need.

Fixes rust-lang#83695, but that's more of a side effect. The upstream compiler already suggests the right method name here.
@bors
Copy link
Contributor

bors commented Apr 11, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 11, 2024
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@estebank
Copy link
Contributor

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2024
@bors
Copy link
Contributor

bors commented Apr 11, 2024

⌛ Testing commit 3a2a3ae with merge f13f37f...

@bors
Copy link
Contributor

bors commented Apr 11, 2024

☀️ Test successful - checks-actions
Approved by: estebank
Pushing f13f37f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2024
@bors bors merged commit f13f37f into rust-lang:master Apr 11, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 11, 2024
@kadiwa4 kadiwa4 deleted the suggest_convert_ptr_to_mut_ref branch April 11, 2024 07:13
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f13f37f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [1.0%, 1.5%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.3% [1.0%, 1.5%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.971s -> 676.139s (-0.12%)
Artifact size: 316.01 MiB -> 316.07 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-raw-pointers Area: raw pointers, MaybeUninit, NonNull A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion for as_mut_ref method on *mut T is <*const T>::as_ref rather than <*mut T>::as_mut
8 participants